-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add collection_is_never_read
#10415
Add collection_is_never_read
#10415
Conversation
r? @giraffate (rustbot has picked a reviewer for you, use r? to override) |
r? @llogiq giraffate seems to be busy and you seem to know about this lint based on the description. You are welcome to reassign as well :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general direction. The code in general looks OK, I have only a small suggestion. However, I'd like to see more tests and perhaps docs about possible known problems.
Thanks for your comments. Regarding docs about possible known problems: I honestly can't think of any. Using |
I came up with a false positive: The lint assumes that a statement I'm not aware of any methods like that in the container types I'm targeting, but such methods can be added using extension traits: trait VecExt<T> {
fn print_self(&self);
}
impl<T> VecExt<T> for Vec<T> {
fn print_self(&self) {
println!("my length: {}", self.len());
}
}
let mut x = vec![1, 2, 3]; // Wrong warning: `collection_is_never_read`
x.print_self(); Perhaps I should check if Is there a way to check where |
Regarding the possible false positive via extension trait, if the trait is defined in the local crate, we might be able to get the method by def_id. I would advise to avoid that because it only pushes the boundary one step: If your trait method calls another one, you're back to step one unless you can follow the whole chain of methods, at which point you will run the risk of a) a general performance nightmare and b) possible infinite recursion if methods call themselves. Even if you collect all the needed data on So I would suggest we add that possibility under |
My idea is simpler than what I think you describe: I don't want to look into the definitions of any methods to see if they have side effects. I only want to identify calls to methods that are not part of the "official" collection type. Such calls can be pessimistically considered reads. This avoids the false positive above (but might create false negatives, which seems acceptable). I think I can identify such methods via the orphan rule: Any method on, say, I've pushed a commit that implements this idea. What do you think? |
In that case, you'd need to check for all kinds of unknown external methods taking the collection as an argument, right? The current commit only checks for extension trait methods. E.g. if mycrate defines |
How the lint works: For a local variable The only way of accessing In your example, fn foo<T>(v: &Vec<T>) -> usize {
v.len()
}
let x = vec![1, 2, 3]; // Ok, no warning
foo(&x); Note that this particular example could be considered a false negative because nothing useful is ever done with the vector. But that's beyond the scope of the lint. If the collection is passed to a function (local or not), then the lint assumes that that's meaningful and necessary. It does not try to look into the method. Since |
Ah, I missed that. Thank you for writing this lint! @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Thanks! A few things I was hoping to discuss before merging:
|
Fixes #9267
@flip1995 and @llogiq, I talked with you about this one at Rust Nation in London last week. :-)
This is my first contribution to Clippy, so lots of feedback would be greatly appreciated.
.stderr
file)cargo test
passes locallycargo dev update_lints
cargo dev fmt
dogfood
found one true positive (see #9509) and no false positives.lintcheck
found no (true or false) positives, even when running on an extended set of crates.changelog: new lint [
collection_is_never_read
]#10415